COMP: Resolve Python ST/IT/OT from the wrapped C types - #6772

Shouldn't this identifier start with an underscore (as in _ITK_GLOBAL_WRAPPING_TYPE_ALIASES), to indicate that it is only for internal use?

I'd lean toward keeping it un-prefixed, for consistency with its two immediate siblings in this same file — ITK_GLOBAL_VERSION_STRING (line 182) and ITK_GLOBAL_WRAPPING_BUILD_OPTIONS (line 185) — both of which are equally internal and neither of which is underscore-prefixed.

The established convention here looks like it puts the privacy marker on the consumer side rather than the definition side, e.g. itk/support/build_options.py:

from itkConfig import ITK_GLOBAL_WRAPPING_BUILD_OPTIONS as _itkwrapbo

That is what the as _wrapping_type_aliases in this PR was imitating — but since you and Dzenan both preferred a single name for the dict, I have dropped the rename and instead del the imported name after use, so nothing leaks into itk.support.types either way.

Happy to go the other direction if you'd rather: either underscore-prefix this one alone (accepting the inconsistency), or rename all three ITK_GLOBAL_* names in a separate STYLE: commit so the file stays uniform. Your call — just say which and I'll make the change.

I find the use of two dictionaries here a bit complicated. It may make the code harder to understand for human readers. Would it be possible to simplify these three lines to just:

ST = uint64_t if ITK_USE_64BITS_IDS else uint32_t
IT = ST
OT = int64_t if ITK_USE_64BITS_IDS else int32_t

Then we would only need to make ITK_USE_64BITS_IDS accessible in Python, right? (Assuming ITK_USE_64BITS_IDS is still needed, of course, as discussed at issue #6774.)

TLDR: I'm not willing to hold up the fix based on a nit like this, and since either method works, I'll do you preference in a forced push in a few minutes.

====
I have a minor preference disagreement with the proposed change request. The double dictionary lookup is a common way to map between types, and I think it more clearly defines what we are trying to accomplish. I think that this paradigm will be more maintainable in the future.

The conditional based on a compile-time option that we are considering removing seems like a move in the wrong direction.

Reversing what I said earlier — after checking it against a real build, I'm keeping the dictionary lookup as written. Your suggestion misses that the wrapped types do not follow ITK_USE_64BITS_IDS alone; the selection is gated on WIN32 as well, so the proposed form is only correct on Windows.

Wrapping/WrapBasicTypes.cmake:220-235 is the authority:

# Types that correspond itk::SizeValueType, itk::IdentifierType, and itk::OffsetValueType
if(WIN32 AND ITK_USE_64BITS_IDS)
  set(ITKM_ST ${ITKM_ULL})   # ITKM_IT ULL, ITKM_OT SLL
else()
  set(ITKM_ST ${ITKM_UL})    # ITKM_IT UL,  ITKM_OT SL
endif()

On macOS and Linux the WIN32 term is false, so the else branch is always taken and ST/IT are always UL — 64-bit there — regardless of how ITK_USE_64BITS_IDS is set. This matches the platform table in #6774: on LP64 the option is a no-op in both positions, because the guard at itkIntTypes.h:65 additionally requires ULLONG_MAX != ULONG_MAX. It changes types only on Windows LLP64 and on 32-bit targets such as WebAssembly.

Checked against my macOS build, which has ITK_USE_64BITS_IDS:BOOL=OFF (/* #undef ITK_USE_64BITS_IDS */ in the generated itkConfigure.h):

So it would break the default macOS/Linux configuration — where nearly all wheels are built — and on Windows/OFF it would still not produce the right answer, just a different wrong one.

There is a second, subtler problem: uint32_t is UI (unsigned int), not UL. Even where the widths coincide these are distinct itkCTypes with distinct mangled names, so itk.ST would name an instantiation the wrapping never created — which is the class of bug this PR exists to fix.

Worth noting for the record that this would be the third independent re-derivation of a value CMake already computes, each wrong in its own way: the original if os.name == "nt": ST = ULL ignored the option entirely; #6767 (b5eb3ae3ffb) replaced it with an unconditional uint64_t; and this proposal drops the WIN32 term. Making your version correct would mean restoring the os.name == "nt" check you removed in #6767. That history is the argument for reading ITKM_* rather than reconstructing it — the dictionary is doing real work, not ceremony.

I take your point that two dictionaries read as indirection. I'd rather keep the mapping explicit than trade correctness for brevity here, so I'm leaving the three lines as they are. Happy to revisit the spelling if you see a form that keeps the CMake value as the single source of truth.

Do you mean that the following should work?

ST = uint64_t if os.name != "nt" || ITK_USE_64BITS_IDS else uint32_t
IT = ST
OT = int64_t if ST == uint64_t else int32_t

Then for now I think that would be OK.

Oh, I see now, it wants UL for Windows + OFF 🤔 So then:

ST = uint64_t if os.name != "nt" || ITK_USE_64BITS_IDS else UL
IT = ST
OT = int64_t if ST == uint64_t else SL

This is the type of logic that we want to avoid. It's replicating the logic that is already hard-coded in the dictionaries and can not deviate from what occurs in the C++ layer. The current solution I have will track what the C++ layer demands without the need for manual synchronization. Your solution requires manual modification of this logic for every change in the CMake or C++ logic.